Skip to content
This repository has been archived by the owner on Oct 2, 2020. It is now read-only.

Added MCP4251 #2169

Merged
merged 25 commits into from
Oct 5, 2019
Merged

Added MCP4251 #2169

merged 25 commits into from
Oct 5, 2019

Conversation

JeppeSRC
Copy link
Contributor

@JeppeSRC JeppeSRC commented Sep 16, 2019

Added MCP4251

Datasheet

image

I ended up creating 3 different symbols for PDIP, SOIC, TSSOP packages even if they have the same pinout, because the QFN package didn't have the same pinout.


All contributions to the kicad library must follow the KiCad library convention

Thanks for creating a pull request to contribute to the KiCad libraries! To speed up integration of your PR, please check the following items:

  • Provide a URL to a datasheet for the symbol(s) you are contributing
  • An example screenshot image is very helpful
  • Ensure that the associated footprints match the official footprint library
    • A new fitting footprint must be submitted if the library does not yet contain one.
  • If there are matching footprint PRs, provide link(s) as appropriate
  • Check the output of the Travis automated check scripts - fix any errors as required
  • Give a reason behind any intentional library convention rule violation.

@CLAassistant
Copy link

CLAassistant commented Sep 16, 2019

CLA assistant check
All committers have signed the CLA.

@cpresser cpresser added Addition Adds new symbols to library Pending reviewer A pull request waiting for a reviewer labels Sep 18, 2019
@cpresser
Copy link
Contributor

cpresser commented Sep 18, 2019

Hi and thanks for the PR.

To bad KiCad does not support different footprints for aliases. That would have saved you 2 symbols.

It looks like there is enough space in the symbol-body to add resistor symbols. Similar to other digital pots already in the library:
image

Those are the issues/questions found so far:

  • Add resistor symbols (check if it looks good and fits)
  • Description should be separated by commas
  • The 'E' in the name should also be replaced by a 'x' (see klc S2.2)
  • -ML Part: Add Pin17, Stack all VSS pins
  • -ML Part: Description says QFN-14, but it is QFN-16
  • -ML Part: The Package has a EP of 2.65mm(nominal), the recommended landing-pattern has no nominal value, just a max of 2.5mm. There are quite a lot of footprints in the library that are a close fit, but none is exact:
    image

Assuming we put the EP-Size of 2.8mm into the IPC footprint-generator it will generate a exposed pad of 2.8x2.8mm, which does violate the Microchip data-sheet. Not sure how to handle this.

@cpresser cpresser self-assigned this Sep 18, 2019
@cpresser cpresser added Pending changes User is expected to perform fixes before merging and removed Pending reviewer A pull request waiting for a reviewer labels Sep 18, 2019
@chschlue
Copy link
Contributor

@cpresser
Keywords should be separated by spaces.

@cpresser
Copy link
Contributor

@cpresser
Keywords should be separated by spaces.

Thanks for the hint; I meant to write Description - Above post was edited.

@chschlue
Copy link
Contributor

The current ML package spec (http://ww1.microchip.com/downloads/en/DeviceDoc/16L_QFN_4x4_ML_C04-0127D.pdf) still gives the same dimensions.
I think that leaves us with 2.5x2.5mm (max recommended land and min package pad).

@JeppeSRC
Copy link
Contributor Author

JeppeSRC commented Sep 19, 2019

So do we stick with the 2.1mm or do we create a 2.5mm?

@chschlue
Copy link
Contributor

See KLC F6.3

EP may vary within MF recommendations but must at least be minimum package pad size.

@JeppeSRC
Copy link
Contributor Author

So a new footprint it is.

@cpresser
Copy link
Contributor

So a new footprint it is.

Yep. As @chschlue did point out. KLC F6.3 is quite specific:

It is not allowed to use a footprint pad smaller than the minimum size of the package pad.

But you can use the generator scripts for that, making it quite easy to create a new FP. Just copy one of the existing ones in the .yaml file and change the EP size. Also check the other values so they are the same as in the data-sheet.
https://github.com/pointhi/kicad-footprint-generator
Please link both the footprint PR and the generator PR to this one.

Please also note that there were quite a few other things that need to be changed before we can merge it. The checklist is in my first post. I will do another check once you addressed those points.

@cpresser cpresser added the Pending footprint Pending footprint acceptance before merging label Sep 21, 2019
@JeppeSRC
Copy link
Contributor Author

@cpresser I've fixed the issues in the checklist and here are the PRs for the footprint.

Generator PR: pointhi/kicad-footprint-generator#430
Footprint PR: KiCad/kicad-footprints#1870

@cpresser
Copy link
Contributor

cpresser commented Sep 23, 2019

I like how you handled each comment in a separate commit. That makes reviewing them really simple. Yay!

However, I am not yet happy with the VSS pins. They should all be stacked.
image

The KLC rule is S4.3. In this case the following changes will do the trick:

  • Make Pin3 visible and power input
  • Make Pin4 invisible and passive
  • Rename Pin17 to Vss, set to passive and stack with 3 & 4 (Edited, see below)

@evanshultz
Copy link
Collaborator

Also the FP filters need a little work:

  1. Remove everything before the colon.
  2. Do not include pin count.
  3. Do not include the EP size.
  4. Use asterisks instead of question marks where possible (everywhere in this PR).

@cpresser
I get pins 3 and 4 stacked, but do you mean pin 17 should be a separate pin instead of pin 14? Pin 14 is SDO. By having pin 17 separate the user can float it as the datasheet allows, but stacking all three pins does not and the user might want to do that for layout reasons. To make it clear which of the two bottom-side pins are which, the pin 3/4 stack would be named VSS and pin 17 would be named EP (I don't know any rule for this pin's name but I recall seeing whatever the datasheet calls it being common). Let me know if I'm mistaken or confused.

@cpresser
Copy link
Contributor

cpresser commented Sep 23, 2019

@cpresser
I get pins 3 and 4 stacked, but do you mean pin 17 should be a separate pin instead of pin 14? Pin 14 is SDO. By having pin 17 separate the user can float it as the datasheet allows, but stacking all three pins does not and the user might want to do that for layout reasons. To make it clear which of the two bottom-side pins are which, the pin 3/4 stack would be named VSS and pin 17 would be named EP (I don't know any rule for this pin's name but I recall seeing whatever the datasheet calls it being common). Let me know if I'm mistaken or confused.

You are correct, there was a typo (14, meant 17). Fixed it in the post above. /me should be more carefull.

Regarding the stacking of the EP: Afaik other parts in this lib also stack EP even if it is just recommended. My argument is also that nothing else can be connected to EP but GND. So the user does not really have any freedom on what else to connect/route to the center pad.
Thats why I suggested that.

Nevertheless, the non-stacking variant is also valid. In this case Pin17 should be an invisible NC pin, so the user can't connect it to other nets.

@evanshultz
Copy link
Collaborator

Pin 17 should be a separate pin, visible, and on the bottom next to the pin 3/4 stack. The user can then do three things with the thermal pad:

  1. Connect it to the same net as the VSS pins.
  2. Leave it floating.
  3. Connect it to another net. Maybe a different ground, which could possibly mess things up with noise or maybe the VSS pins use a net tie to the thermal pad (which is connected to system ground) in order to force a particular routing scheme.

This library may be a bad example (one of many!) but this is done elsewhere to give the user all possibilities without forcing a connection that they may not want.

@cpresser
Copy link
Contributor

@evanshultz I get your points. And they are totally valid. If the goal is to have every option to the user than that is the way to go.
I personally prefer it the other way (less options = less opportunities for mistakes), but that is rater off-topic in this PR.

What do you think the pin-type for Pin17 EP should be? passive? That might give the least ERC errors. I can't find a KLC rule for this special case. Its neither a hidden nor a NC pin.

Do you think we should clarify the KLC for this (not so) special case?

@JeppeSRC please do as suggested by evanshultz.

@evanshultz
Copy link
Collaborator

Yes, pin 17 should be Passive type.

This is the only way I can recall it being done for years since I started and have seen PRs reviewed. I'm sure there are many symbols that don't conform, and v6 would be the next opportunity to revamp them. Having KLC capture this and an issue to track naughty symbols would be nice.

@cpresser
I like fewer options and the most straightforward too, but we can't be sure the proclivities of all users of this library so we should be flexible.

@cpresser
Copy link
Contributor

In order to fix that travis-CI error you should change the pin-type of pin11 and also move it onto the boundary of the symbol body like so:

@chschlue chschlue removed the Pending footprint Pending footprint acceptance before merging label Sep 28, 2019
@cpresser cpresser merged commit d3cbfd0 into KiCad:master Oct 5, 2019
@cpresser cpresser removed the Pending changes User is expected to perform fixes before merging label Oct 5, 2019
@antoniovazquezblanco antoniovazquezblanco added this to the 5.1.5 milestone Oct 7, 2019
@JeppeSRC JeppeSRC deleted the mcp4251 branch December 18, 2019 19:00
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Addition Adds new symbols to library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants